Skip to content

Conversation

335g
Copy link

@335g 335g commented Sep 9, 2025

Which issue does this PR close?

What changes are included in this PR?

  • Introduced a new SqlCatalogBuilder implementation for iceberg::CatalogBuilder.
  • By transitioning the builder implementation from SqlCatalogConfig to SqlCatalogBuilder, we've simplified the process for creating SqlCatalog objects.
  • Enabled generation of Box<dyn BoxedCatalogBuilder> for the SqlCatalog via the load function (Implement catalog loader function. #1260)

Are these changes tested?

Yes (only tested whether it could be made)

@335g
Copy link
Author

335g commented Sep 22, 2025

The implementation is complete and I'd like you to review it. 🙇

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @335g for this pr! Left some comments to resolve.

@335g
Copy link
Author

335g commented Sep 23, 2025

@liurenjie1024 Thanks for the review! I've addressed the comments you made. 🙇🏼

uri: "".to_string(),
name: "".to_string(),
warehouse_location: "".to_string(),
file_io: FileIOBuilder::new_fs_io().build().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not panic here, instead we should report an error.

@335g
Copy link
Author

335g commented Sep 27, 2025

@liurenjie1024 May I ask a question?
Currently, to comply with iceberg::CatalogBuilder, implementing Default is required. Why exactly is Default necessary? I understand it's needed for the load function in the iceberg-loader-catalog crate, but are there any other reasons?

SqlCatalog requires iceberg::FileIO, but implementing Default to allow users to select the storage location proves challenging. Currently, I'm considering implementing it by passing iceberg::FileIO as an argument to the initializer.

impl SqlCatalogBuilder {
    pub fn new(file_io: FileIO) -> Self {
        ...
    }
}

If possible, I'd like to remove Default from the iceberg::CatalogBuilder definition.

@liurenjie1024
Copy link
Contributor

Hi, @335g I'm not convinced that we should remove Default for CatalogBuilder, as the default method provides a default constructor for it.

SqlCatalog requires iceberg::FileIO, but implementing Default to allow users to select the storage location proves challenging.

It's CatalogBuilder that requires implementing Default, not SqlCatalog. I took a look at the code and I think storing FileIO in SqlCatalogConfig is incorrect, FileIO should be inferred from warehouse location.

@335g
Copy link
Author

335g commented Sep 30, 2025

Hi, @liurenjie1024 Thanks for the comment. I realized I didn't fully understand the previous comment. I've made some adjustments to the implementation - how does this look?

@335g
Copy link
Author

335g commented Oct 3, 2025

Resolved conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement catalog loader for sql

2 participants